mutating webhook for defaulting storageclass params #3055
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: leelavg The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
leelavg
force-pushed
the
sc-hook
branch
3 times, most recently
from
892e7b9 to
a83feba
Compare
Signed-off-by: Leela Venkaiah G <[email protected]>
Signed-off-by: Leela Venkaiah G <[email protected]>
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
For external storageclasses the assumption was it already has details filled and webhook simply don't mutate any of them. |
Signed-off-by: Leela Venkaiah G <[email protected]>
| Expression: fmt.Sprintf( | ||
| "request.object.provisioner in ['%s', '%s']", | ||
| util.RbdDriverName, | ||
| util.CephFSDriverName, |
There was a problem hiding this comment.
a separate PR should follow for NFS, I can't send PR for something I didn't try out.
| Operations: []admrv1.OperationType{admrv1.Create}, | ||
| }, | ||
| }, | ||
| FailurePolicy: ptr.To(admrv1.Fail), |
There was a problem hiding this comment.
wont having FailurePolicy will have sideeffect on other classes if the webhook server is down? can you please check on confirm on it?
There was a problem hiding this comment.
it'll but while setting webhook I'm using match conditions on our provisioners only, if you are referring non-odf sc as other they are exempted.
There was a problem hiding this comment.
what will be the impact on the SC for external mode?
| var webhookService = corev1.Service{ | ||
| Spec: corev1.ServiceSpec{ | ||
| Ports: []corev1.ServicePort{ | ||
| { | ||
| Name: "ocs-operator-webhook", | ||
| Port: webhookServicePort, | ||
| Protocol: corev1.ProtocolTCP, | ||
| TargetPort: intstr.FromInt32(WebhookServiceTargetPort), | ||
| }, | ||
| }, | ||
| Selector: map[string]string{ | ||
| "name": "ocs-operator", | ||
| }, | ||
| Type: corev1.ServiceTypeClusterIP, | ||
| }, | ||
| } | ||
|
|
||
| var storageClassMutatingWebhook = admrv1.MutatingWebhook{ |
There was a problem hiding this comment.
do we need service and webhook created as part of the code not part of yaml files in the CSV? can you please specify the reason and pros and cons?
There was a problem hiding this comment.
svc:
I want to ensure the annotation even if it's removed by mistake and having the svc in bundle doesn't ensure it
webhook:
I need support for setting MatchConditions which aren't exposed on CSV, need typed structs + enforcement and so not part of bundle
|
|
||
| var supportedProvisioners = []string{ | ||
| util.RbdDriverName, | ||
| util.CephFSDriverName, |
There was a problem hiding this comment.
a separate PR should follow for NFS, I can't send PR for something I didn't try out.
| clusterID := "" | ||
| for idx := range storageConsumerList.Items { | ||
| consumer := &storageConsumerList.Items[idx] | ||
| if consumer.Annotations[defaults.StorageConsumerTypeAnnotation] == defaults.StorageConsumerTypeLocal { |
There was a problem hiding this comment.
what happens if i create a storageconsumer and add local annotation. no one will block me from doing that isnt it?
There was a problem hiding this comment.
yes, either this or an immutable field in the spec, we went w/ the annotation as not to backport spec changes.
let's discuss in upcoming meetings if any change is required.
controllers/webhook/storageclass.go
Outdated
| if storageClass.Parameters["csi.storage.k8s.io/provisioner-secret-name"] != "" { | ||
| patches = append(patches, jsonpatch.JsonPatchOperation{ | ||
| Operation: "add", | ||
| Path: "/parameters/csi.storage.k8s.io~1provisioner-secret-name", |
There was a problem hiding this comment.
csi.storage.k8s.io~1provisioner-secret-name typo in this one
There was a problem hiding this comment.
i felt it as typo, can you please add a comment for this one :(
Signed-off-by: Leela Venkaiah G <[email protected]>
|
@leelavg: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold Will squash after final review |
openshift-ci
bot
added
the
do-not-merge/hold
webhook for populating secret names and clusterid in storageclass parameters for supported provisioners based on local storageconsumer.